Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix compatibility with cartopy 0.20.0+ #378

Merged
merged 2 commits into from
Sep 30, 2021

Conversation

djhoese
Copy link
Member

@djhoese djhoese commented Sep 23, 2021

Cartopy 0.20.0 includes updates by @snowman2 so that cartopy's CRS objects are now a subclass of pyproj's CRS class. Somehow this release broke compatibility with pyresample's hacky cartopy conversion where we started getting errors in our tests related to towgs84 PROJ.4 string parameters being invalid. Something got lost in the CRS -> proj4 dict -> proj4 str -> CRS conversion.

Rather than figure out where this conversion issue was happening, I chose to implement something that I discussed in the cartopy issue here (SciTools/cartopy#813). Instead of tricking cartopy into accepting arbitrary PROJ.4 information, this uses the new pyproj/cartopy CRS functionality to pass pyresample's pyproj CRS object directly to cartopy. This PR acts as a test for this functionality being added directly to cartopy (see referenced issue).

  • Tests added
  • Tests passed

@djhoese djhoese added the bug label Sep 23, 2021
@codecov
Copy link

codecov bot commented Sep 23, 2021

Codecov Report

Merging #378 (a1a7e9e) into main (60440ca) will decrease coverage by 0.13%.
The diff coverage is 56.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #378      +/-   ##
==========================================
- Coverage   93.54%   93.40%   -0.14%     
==========================================
  Files          50       50              
  Lines       10436    10405      -31     
==========================================
- Hits         9762     9719      -43     
- Misses        674      686      +12     
Flag Coverage Δ
unittests 93.40% <56.66%> (-0.14%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pyresample/utils/cartopy.py 59.25% <52.63%> (-35.48%) ⬇️
pyresample/geometry.py 86.34% <55.55%> (-0.31%) ⬇️
pyresample/test/test_geometry.py 99.27% <100.00%> (-0.01%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60440ca...a1a7e9e. Read the comment docs.

@coveralls
Copy link

coveralls commented Sep 23, 2021

Coverage Status

Coverage decreased (-0.1%) to 93.396% when pulling a1a7e9e on djhoese:bugfix-cartopy-compat into 60440ca on pytroll:main.

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes lgtm, however, the new cartopy 0.20 isn't tested, right?

@djhoese
Copy link
Member Author

djhoese commented Sep 23, 2021

It is tested. That's how I knew that things weren't working (my other PRs were failing). This does not explicitly test older versions of cartopy, but I think given the improvements that are going on in cartopy we push users to update to newer cartopy as soon as possible. I'm not saying that old cartopy versions won't work with pyresample, but I would consider them on a short support cycle for the pyresample functionality. We can remove a lot of code from pyresample after my PR in cartopy gets merged and released.

Here's the conda log from one of the ubuntu jobs run here:

    + cartopy                              0.20.0  py38hc951a7f_0       conda-forge/linux-64       2 MB

Otherwise, you requested changes?

@mraspaud
Copy link
Member

OK, I don't understand the how you can test that both the new and old cartopy support is working without changing the tests more...

@djhoese
Copy link
Member Author

djhoese commented Sep 24, 2021

As described on slack, it isn't really possible to test both old and new cartopy. Eventually we want to deprecate old cartopy version support so we (@mraspaud and I) decided it would be easier to do it now. This PR now fixes support for 0.20.0+ but removes support for old versions. This cleans out a lot of old code. If my new cartopy PR gets merged and released then we could remove even more code from pyresample and just use cartopy directly.

@djhoese
Copy link
Member Author

djhoese commented Sep 24, 2021

@mraspaud I just found out from a student employee that the URL that cartopy uses for downloading shapefiles from NaturalEarth was updated for 0.20.0. The old one doesn't work any more. So forcing users to update to 0.20.0 doesn't sound so bad.

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mraspaud mraspaud merged commit e9d632d into pytroll:main Sep 30, 2021
@djhoese djhoese deleted the bugfix-cartopy-compat branch September 30, 2021 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants